Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(bundling): upgrades deps and bundling #130

Merged
merged 5 commits into from
Jan 30, 2024

Conversation

dysfunc
Copy link
Contributor

@dysfunc dysfunc commented Jan 25, 2024

This includes:

  • Upgrades storybook to 7.6.10
    • Updates the storybook config to use the latest
  • Upgrades vite to 5.0.12
  • Upgrades vitest to 1.2.1
  • Upgrades eslint, and related plugins
  • Upgrades svelte to 4.2.9
  • Upgrades prettier to 3.0.1
  • Updates CI scripts to use Node 20
  • Upgrades @sveltejs/package and includes publint for linting bundles.
    • Bundles now build to dist
  • Includes the correct exports in package.json

I've tested the new bundling with my company's production web app, and it works as expected. Please do your own testing as well :)

This was the initial PR #128 @niemyjski

@dysfunc dysfunc changed the title chore(bundling): upgrades storybook, vite, vitest and bundling chore(bundling): upgrades storybook, svelte, eslint, vite, vitest and bundling Jan 27, 2024
@dysfunc dysfunc changed the title chore(bundling): upgrades storybook, svelte, eslint, vite, vitest and bundling chore(bundling): upgrades deps and bundling Jan 27, 2024
Copy link
Collaborator

@niemyjski niemyjski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the pr! I left some comments below

.eslintrc.json Outdated Show resolved Hide resolved
.eslintrc.json Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
src/Chart.svelte Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
vite.config.js Outdated Show resolved Hide resolved
vite.config.js Show resolved Hide resolved
@dysfunc dysfunc force-pushed the bundling branch 2 times, most recently from 7bd3c7c to e46c064 Compare January 29, 2024 16:50
"settings": {
"svelte3/typescript": true
"import/no-unresolved": 0,
"no-inner-declarations": 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this one was required so there weren't a ton of errors introduced?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was only one error, which was the one I rolled back. I figured we could always fix it later and remove the lint rule.

@niemyjski
Copy link
Collaborator

@dysfunc Can you please fix the lock file as the build is failing.

Might be good to update the actions as well as I think the latest story book requires node 18+. Would just be good to target LTS (v20)

@niemyjski
Copy link
Collaborator

@dysfunc I think we might need to upgrade npm actions to node 20, I can't even run master branch locally due to esm failures.

@dysfunc
Copy link
Contributor Author

dysfunc commented Jan 30, 2024

@dysfunc I think we might need to upgrade npm actions to node 20, I can't even run master branch locally due to esm failures.

What errors are you seeing? I can install, build, run storybook on 16+

@niemyjski
Copy link
Collaborator

There were precommit hooks were failing for me with a lot of esm related messages. Looks like two of the github actions are failing, one of them with esm message. Thanks again for all of your help. I think the deps being so out of date is making this harder than normal. That will be the next thing I handle after this pr

@dysfunc
Copy link
Contributor Author

dysfunc commented Jan 30, 2024

I fixed the size test. The only one left is the editorconfig. It's failing on .git directory which should be ignored.

@niemyjski
Copy link
Collaborator

Thanks, I'm going to merge this and then fix that in a separate pr.

@niemyjski niemyjski merged commit 31577ff into SauravKanchan:master Jan 30, 2024
3 of 4 checks passed
@dysfunc dysfunc deleted the bundling branch January 30, 2024 15:27
@niemyjski
Copy link
Collaborator

@dysfunc just noticed this

https://github.com/SauravKanchan/svelte-chartjs/actions/runs/7716442819/job/21033255322

All good!
 ERR_PNPM_NO_IMPORTER_MANIFEST_FOUND  No package.json (or package.yaml, or package.json5) was found in "/home/runner/work/svelte-chartjs/svelte-chartjs/dist".
Error: Process completed with exit code 1.

I can't think of anything off the top of my head that would cause this other than possibly the old process was outputting a package file?

@dysfunc
Copy link
Contributor Author

dysfunc commented Jan 30, 2024

the old process did copy package.json. the new process doesn't; it uses the root package.

@niemyjski
Copy link
Collaborator

This is the first time I've used pnpm, kinda looks like we need to copy it.

@dysfunc
Copy link
Contributor Author

dysfunc commented Jan 30, 2024

it shouldn't be copied over. I'm looking at it right now

@dysfunc
Copy link
Contributor Author

dysfunc commented Jan 30, 2024

i think the publishConfig might be the issue. that can be removed

  "publishConfig": {
    "directory": "dist"
  },

@niemyjski
Copy link
Collaborator

It used to be there for the package folder?

"publishConfig": {
    "directory": "package"
  },

@dysfunc
Copy link
Contributor Author

dysfunc commented Jan 30, 2024

I would remove that and rerun publish

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants